Optional cached imports - #72
kristoffer-gustafsson wants to merge 7 commits into
Conversation
This test checks that some warning is logged when multiple entry points are found for one module. However, this part of the code is never reached if on the second call with reuse_cached_imports = True instead of False. That is because the entrypoints are cached in the previous call, which skips _load_from_entrypoints where the warning is raised. Therefore ymmsl_cache is cleared before and after each test to ensure a clean slate. The failed test:: FAILED ymmsl/v0_2/tests/test_resolver.py::test_resolve_entrypoints_duplicate_name[True] - assert 0 == 1 + where 0 = len([])
LourensVeen
left a comment
LourensVeen
left a comment
There was a problem hiding this comment.
Thanks, that's a good addition. I'd like to simplify the test a bit, as indicated. And could you fix the formatting so the CI passes?
|
|
||
| def test_apply_custom_implementations_everything_localised() -> None: | ||
| def test_apply_custom_implementations_everything_localised( | ||
| env_ymmsl_path: None, resolve: Resolve |
There was a problem hiding this comment.
Why did env_ymmsl_path get added here?
There was a problem hiding this comment.
Before I believe it was not needed because the cache was not cleared between tests, leaving test_model at a/e.ymmsl in the cache from earlier tests. Now that the cache is cleared between each test, test_apply_custom_implementations_everything_localised needs to run the env_ymmsl_path fixture so that it may resolve its import "from a.e import implementation test_model" on its own.
|
|
||
|
|
||
| @pytest.fixture(params=[False, True]) | ||
| def resolve(request: pytest.FixtureRequest) -> Resolve: |
There was a problem hiding this comment.
I would prefer it if this just returned the boolean parameter, and the tests passed it explicitly. That way, when reading the test, you can see what's going on, and the resolve function that's being called is actually what you think it is.
2 participants
The ymmsl_cache assumes that the YMMSL_PATH and sys.path does not change between resolve calls, but in fact it can.
If it does, the cached module imports can become stale.
Therefore I have added this contract explicitly in the doc-string of the resolve method, along with an option to clear the cache before resolving.